-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Polish Lua examples #2846
Polish Lua examples #2846
Conversation
@galjonsfigur I've removed the DMN tag because I disagree, but because it does matter, IMO, I also think that we should get this right. I'll put some specific review comments inline, but I also think that there is a more general issue that we need to consider / discuss and to agree the broad principles here. This relates to the general issue of how coding in NodeMCU Lua is different and should additional quality criteria should apply? Put simply, I think that Lua for ESP devices should conform to Lua best practice, but not all code that is best practice in a The main resource that we are limited by in our IoT apps is RAM space, so:
The mark and sweep Lua GC is very good at collecting dead data, but the Achilles heal here is the Lua Registry. You will quite often create some object such as a network socket where you then establish Lua callbacks and these CBs may need to have the UData object as an upvalue, hence we have a UData in the registry which refers to a Lua function in the registry which has the UData as an upval. Because the dependency graph of collectible object involves the Registry, this frustrates the Lua GC. You can get similar issues in Lua code, so I feel that any "good" Lua module should include some form of close then results in the complete cleanup of resources. You can see that I do this with my FTPserver and telnet examples as well as the new coroutining one that I am about to add. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that the below seems like a knit-pikck, but if we want developers to use this facility then it should be usable and not slow down the travis check process unduly.
.travis.yml
Outdated
@@ -29,3 +29,8 @@ script: | |||
- if [ "$OS" = "linux" ]; then bash "$TRAVIS_BUILD_DIR"/tools/travis/ci-build-linux.sh; fi | |||
- if [ "$OS" = "windows" ]; then bash "$TRAVIS_BUILD_DIR"/tools/travis/ci-build-windows-ms.sh; fi | |||
- if [ "$OS" = "linux" -a "$TRAVIS_PULL_REQUEST" != "false" ]; then bash "$TRAVIS_BUILD_DIR"/tools/travis/pr-build.sh; fi | |||
- cd "$TRAVIS_BUILD_DIR" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- If Lua checking is mandatory for the linux build then IMO it should be called from
ci-build-linux.sh
and not by extra lines in the YML.
You haven't changed therun-check.sh
on this PR but you should: - If
luacheck
is already on the path then the install components should be skipped. This facilitates developers running the script when developing as well as during Travis QA runs. - Ditto if luarocks is already installed then the only the rocks install of luacheck should be done. As I said in a previous comment we should not be running builds of Lua and Luarocks when they are already available as package addons in the
.travis,yml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those additional commands lines are using luac.cross
to check if the .lua files have no syntax errors and this check is used both in Linux and Windows build - that way also luac.cross
binary can be tested on both platforms.
I modified run-luacheck.sh
to use pre-installed LuaRocks by default, but added an standalone install option just in case. Tested both options and everything seems to work fine (at least in docker environment).
It may be a good option to make this check before building a whole firmware or even better - check if commit changes only .lua files, only C stuff or both (using TRAVIS_COMMIT_RANGE) and run firmware build or luacheck only when necessary but this is out of scope of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might also make sense to have another job for luacheck, just like for windows. That way luacheck could run in parallel and would not require any extra waiting time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HHHartmann I added script to run Luacheck on Windows and it seems to be working properly.
BTW I just realised that I misunderstood your comment and added Luacheck for windows instead of making it as a separate job. Sorry 😄 It runs much faster on Windows though (because only luac.cross
is built in that enviroment) and the script itself can be used in normal Windows environment with Bash included in Git for Windows and wget.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does luacheck for linux yield difrfrent results than for windoes? If not maybe the windoes part would be enough. But then the linux version is not tested and starts rotting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
luacheck
for both Windows and GNU/Linux should give the exact same output, so it's not really necessary to run it in both environments. The only advantage of using Windows is that it's faster to check because there is official pre-compiled binary of luacheck
with built-in Lua 5.3 and other libraries + Travis can cache the binary itself. A good idea would be to look into Travis CI docs and add separate jobs for changes in Lua code and changes in C code, but that's out-of-scope for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So should we use the windows version only here to save time and add the linux version only to a release build to detect when it is broken?
@TerryE Terry what is your opinion on this?
0ba9e21
to
ca82655
Compare
@galjonsfigur can I ask you to resolve the conflicts please. @TerryE any more changes requested from your side? @HHHartmann I think yours were properly addressed, no? |
ca82655
to
94187ed
Compare
@marcelstoer I resolved conflicts and made some updates like adding support for the DCC module. |
Wow, thank you so much! I really appreciate your efforts 👏 |
tools/travis/run-luacheck-windows.sh
Outdated
|
||
#Download luacheck binary if nessesary | ||
if ! [ -x "cache/luacheck.exe" ]; then | ||
wget -O cache/luacheck.exe https://github.com/mpeterv/luacheck/releases/download/0.23.0/luacheck.exe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might make sense to define the retry params. You don't want a network hiccup to fail your build.
Some nitpicking from my side.
But none of them are showstoppers in my opinion. Another issue is what we can do to not forget about the suggestions to further improve the tests by @galjonsfigur and @TerryE. Should we create another Issue for those? |
@HHHartmann Thanks for feedback. I will look into this again and replace rest of The underscore variable is mostly just a Lua community convention to mark variable that is unused or used just as a "padding" to get the desired output from function returning multiple variables. For example: About the globals usage: I think the display example where all those globals are used is pretty interesting - the example code loads and unloads some of the code to save RAM and global variables are used as shared data. I will try to find a better way to do this but for now it's not really bad. |
@galjonsfigur that sounds good. Overall this PR is an importent thing to have. In C if we break an interface the compiler immediately tells us but often we forget the Lua codebase. With LFS this has become even more important. |
I've scanned through the various changes, and I feel that overall the Lua code is much improved. I did come across a couple of instances where @galjonsfigur has included bugfixes in his changes and a couple where the changes also changed the semantics of the source. In one case in some of my old code changing from However, the bottom line to me is that galjonsfigur has done a lot of quality work here and the examples overall will be more solid. We have a general issue that some will be dated because we've since changed APIs etc. and missed regressing this into the examples, but I think it unrealistic to expect to address all these in this PR. One specific note which picks up an earlier point of Gregor's. Often the examples are there as a starting point for developers to build upon and to customise to their particular applications. Hence whilst variables such as A net 👍 |
@TerryE Thank you for the feedback! I think there is a lot more work to do aside from this PR:
And Issues/PRs for each one of them. That's a lot of work and I will try to do them slowly as time allows. |
This seems an excellent, solid improvement. Given @TerryE's +1, I'm going to merge this to dev. Thanks @galjonsfigur! |
* Add missing globals from luacheck config * Fix luacheck warnings in all lua files * Re-enable luacheck in Travis * Speed up Travis by using preinstalled LuaRocks * Fix more luacheck warnings in httpserver lua module * Fix DCC module and add appropriate definitions to luacheck config. * Change inline comments from ignoring block to only ignore specific line * Add Luacheck for Windows and enable it for both Windows and Linux * Change luacheck exceptions and fix errors from 1st round of polishing * Add retry and timeout params to wget
* Add missing globals from luacheck config * Fix luacheck warnings in all lua files * Re-enable luacheck in Travis * Speed up Travis by using preinstalled LuaRocks * Fix more luacheck warnings in httpserver lua module * Fix DCC module and add appropriate definitions to luacheck config. * Change inline comments from ignoring block to only ignore specific line * Add Luacheck for Windows and enable it for both Windows and Linux * Change luacheck exceptions and fix errors from 1st round of polishing * Add retry and timeout params to wget
* Add missing globals from luacheck config * Fix luacheck warnings in all lua files * Re-enable luacheck in Travis * Speed up Travis by using preinstalled LuaRocks * Fix more luacheck warnings in httpserver lua module * Fix DCC module and add appropriate definitions to luacheck config. * Change inline comments from ignoring block to only ignore specific line * Add Luacheck for Windows and enable it for both Windows and Linux * Change luacheck exceptions and fix errors from 1st round of polishing * Add retry and timeout params to wget
* Add missing globals from luacheck config * Fix luacheck warnings in all lua files * Re-enable luacheck in Travis * Speed up Travis by using preinstalled LuaRocks * Fix more luacheck warnings in httpserver lua module * Fix DCC module and add appropriate definitions to luacheck config. * Change inline comments from ignoring block to only ignore specific line * Add Luacheck for Windows and enable it for both Windows and Linux * Change luacheck exceptions and fix errors from 1st round of polishing * Add retry and timeout params to wget
* Add missing globals from luacheck config * Fix luacheck warnings in all lua files * Re-enable luacheck in Travis * Speed up Travis by using preinstalled LuaRocks * Fix more luacheck warnings in httpserver lua module * Fix DCC module and add appropriate definitions to luacheck config. * Change inline comments from ignoring block to only ignore specific line * Add Luacheck for Windows and enable it for both Windows and Linux * Change luacheck exceptions and fix errors from 1st round of polishing * Add retry and timeout params to wget
* Add missing globals from luacheck config * Fix luacheck warnings in all lua files * Re-enable luacheck in Travis * Speed up Travis by using preinstalled LuaRocks * Fix more luacheck warnings in httpserver lua module * Fix DCC module and add appropriate definitions to luacheck config. * Change inline comments from ignoring block to only ignore specific line * Add Luacheck for Windows and enable it for both Windows and Linux * Change luacheck exceptions and fix errors from 1st round of polishing * Add retry and timeout params to wget
Fixes #772.
dev
branch rather than formaster
.docs/*
.This will be a long PR but because it only touches Lua files it shouldn't be hard to rebase if necessary.
Here I will be pushing more and more gradual polishing of the examples. This first pass is only to fix all
luacheck
warnings so it will be easier to work on.And there is a lot to work on:
Blink
sketch - I think it would be a wise idea to do similar thing - add a few examples that would be just as simple only for introduction to development model and maybe to show a proper way to do some basic things like connecting to WiFi. (wifi
module documentation is great but can be unintuitive for a beginner user)yeelink
module is pretty much useless now because it depends on 3rd-party service that doesn't work any more. Other example isirsend.lua
that seems to do some kind of delay to be able to generate proper signal and I doubt if that's still will work after all the changes that were made. But that one is still to be tested.ucglib
,somfy
,bh1750
,dht_lib
, ds3231
,lm92
,hdc1000
are the ones I can't test for now, but testing them will be more important later.I'm still testing if my fixes for
luacheck
warnings broke something and for nowds18b20
,webap_toggle_pin
,adc_rgb
,u8g2
, andpcm/play_file
are confirmed to work - more to come.I encourage anyone to nitpick anything here - this PR is not as important as those that mess with C part of the project and it's a low-priority PR when compared to the others.